Allow overwriting parent metadata resource informations#3724
Conversation
|
IMO it's more a bugfix. It hasn't been detected before because Sylius uses inheritance and extending heavily. Could be a BC break too though... |
|
If it's in a potential BC break, please at least don't merge it in 2.5. It should also be documented. |
@dunglas It can be BC break only in case if someone always had parent without any resource extending it, if right now he want's to extend it he/she must do big copy&paste from parent resource declaration & insert his/her own (full replacement of resource mapping declaration) there to be able to replace parent resource data. From my perspective this is bugfix cause from beginning it had broken logic (parent class winning over child/extending one). But if we're not sure how to proceed with this change, I'm totally okey going (as doing that right now already) against master/2.6-dev branch, locally I'm overloading the whole factory class to fix this bug & most likely will propose that to Sylius, cause developers using that project will be most affected ones. Will try to document this change soon too. Cheers! |
|
Hey @stloyd, in case it will not land in 2.5, we can temporarily host it in Sylius, as internal. But of course, I would prefer not to. Still, thanks for solving this issue 👍 |
|
IMO this is a bug fix as the behavior described in the issue should be the default. I mean we do expect the Child resource to override the parent's one if null (please confirm my sentence). Can you target 2.5 ? I'd like a functional test to illustrate the fix if you can. |
424cca7 to
62f1666
Compare
Currently parent resource data is always being selected if set, while child is silently ignored, this changes the approach to allow extending resource change parent data.
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
5 participants
This is both IMHO bugfix & potentially new feature with BC break (but IMO that BC break is good cause old approach was buggy).
Currently parent resource data is always being selected if set, while child is silently ignored, this changes the approach to allow extending resource change parent data. For example in Sylius predefined resources has
shortName, but if you extend that model & add own@ApiResourceit will ignore the setting you try to use if it was already set on the extended resource.If you think it's correct, I will add tests soon to prove the problem & fix.
ps. not sure if this should land in master or be considered as bugfix & go on 2.5...